Human readable error when selecting wrong security protocol #43
Conversation
|
@gasper-vrhovsek unrecognized command 'bug', ignoring... Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone |
|
@miq-bot add_label bug |
There was a problem hiding this comment.
@gasper-vrhovsek totally a step in the right direction 👍 , but please simplifiy it a bit.
| @@ -92,10 +94,10 @@ def verify_api_credentials(options = {}) | |||
| with_provider_connection(options) {} | |||
| true | |||
| rescue => err | |||
There was a problem hiding this comment.
Please either don't touch the rescue block if you don't need to 😉 or do it properly, because currently it looks like this 🤢 :
rescue => err
_log.error("Error Class=#{err.class.name}, Message=#{err.message}")
miq_exception = translate_exception(err)
raise unless miq_exception
raise miq_exception
endwhich could be simplified into:
rescue => err
_log.error("Error Class=#{err.class.name}, Message=#{err.message}")
raise translate_exception(err)
endThere was a problem hiding this comment.
@miha-plesko i have noticed this block of code was refactored in #40
I think it is best i revert my change here and wait for mergeing of code from the linked PR. Do you agree?
| @@ -47,6 +47,8 @@ def translate_exception(err) | |||
| MiqException::MiqHostError.new "Socket error: #{err.message}" | |||
| when MiqException::MiqInvalidCredentialsError, MiqException::MiqHostError | |||
| err | |||
| when Net::HTTPBadResponse | |||
| Net::HTTPBadResponse.new "Login failed due to a bad hostname and/or port and/or security protocol." | |||
There was a problem hiding this comment.
May I suggest a simple Login failed due to bad security protocol, hostname or port.?
gasper-vrhovsek
force-pushed
the
feature/security_protocol_pretty_error
branch
from
045482d to
8a272b6
Compare
@gasper-vrhovsek Yes, please wait for #40 to get merged. |
|
This pull request is not mergeable. Please rebase and repush. |
gasper-vrhovsek
force-pushed
the
feature/security_protocol_pretty_error
branch
from
8a272b6 to
7e572a4
Compare
gasper-vrhovsek
force-pushed
the
feature/security_protocol_pretty_error
branch
from
7e572a4 to
bff6abe
Compare
|
@gberginc This PR looks good to go. Agree? |
|
@gasper-vrhovsek unrecognized command 'bug', ignoring... Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone |
…rotocol This change modifies the error message returned from nuage when selecting the wrong security protocol. The old error message was: 'wrong status line: "\x15\x03\x03\x00\x02\x02"' which was not informative to the end user. Also added rspec test case to cover this exception.
gasper-vrhovsek
force-pushed
the
feature/security_protocol_pretty_error
branch
from
bff6abe to
888f133
Compare
|
As agreed with @gberginc i have changed the exception type and added a rspec test case which covers the exception tranlsation. |
|
Checked commit gasper-vrhovsek@888f133 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -35,6 +35,8 @@ def translate_exception(err) | |||
| MiqException::MiqHostError.new("Socket error: #{err.message}") | |||
| when MiqException::MiqInvalidCredentialsError, MiqException::MiqHostError | |||
| err | |||
There was a problem hiding this comment.
The problematic error that you mention in the commit msg is returned from the else part otherwise? I am reluctant to use this when case here as it relies on a generic http error message.
Furthermore, do you need to return HTTPBadResponse? In case we need this specific error message, I would still use the same error type as in the else part.
@juliancheal I’ve looked some other providers but couldn’t find any similar error oath - do you think it is necessary?
There was a problem hiding this comment.
@gberginc yes, the problematic error is otherwise returned in the last else statement. And because the error message in the HTTPBadResponse is so poorly written, it has very little meaning showing it to the user through a dashboard error notification.
…_pretty_error Human readable error when selecting wrong security protocol (cherry picked from commit 8ac64a6)
|
Gaprindashvili backport details: |
This change modifies the error message returned from nuage when selecting
the wrong security protocol. The old error message was:
which was not informative to the end user.
@miq-bot bug
/cc @miha-plesko @gberginc